Unified entry point for models and parameter sets #4490
Conversation
santacodes
requested review from
martinjrobins,
Saransh-cpp,
agriyakhetarpal,
kratman,
arjxn-py,
a team,
brosaplanella,
DrSOKane,
rtimms,
valentinsulzer and
TomTranter
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4490 +/- ##
===========================================
- Coverage 99.22% 99.21% -0.01%
===========================================
Files 302 303 +1
Lines 22821 22832 +11
===========================================
+ Hits 22643 22653 +10
- Misses 178 179 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Good start @santacodes, looks like this is still a WIP? Still I have commented just a tip hoping it'd make sense.
There are some conflicts that you may want to resolve and also feel free to add [WIP] prefix to your PR title while you're on it.
src/pybamm/entry_points.py
Outdated
| model_sets = EntryPoint(group="pybamm_models") | ||
|
|
||
|
|
||
| def Model(model: str): # doctest: +SKIP |
There was a problem hiding this comment.
The model function returns an object of an initialised model along with the address of the object. This might vary per instance in a personal computer so the doctests couldn't assert the example I have mentioned in docstring. For example something like <pybamm.models.spm.SPM object 0x7381> is returned when the Model() function is called, the address might be different across various systems which would cause the doctests to fail. In brief I think doctesting and asserting two objects of a class would be a bad idea, unless there's a workaround for it. Do let me know if there is, I'm happy to implement it! :)
src/pybamm/entry_points.py
Outdated
| model_sets = EntryPoint(group="pybamm_models") | ||
|
|
||
|
|
||
| def Model(model: str): # doctest: +SKIP |
There was a problem hiding this comment.
Would Model() as a method be sufficient? My idea is that a class would add more utility and you can have similar required methods as parameter sets i.e. get_docstring or other model operations.
There was a problem hiding this comment.
All of the parameter_sets function also works for models. The Model() method is just an addition to that. As I mentioned this is a unified entry point for both models and parameter-sets. They operate on the same API, just different instances for models and parameter-sets.
santacodes
changed the title
src/pybamm/entry_points.py
Outdated
| parameter_sets = EntryPoint(group="pybamm_parameter_sets") | ||
|
|
||
| #: Singleton Instance of :class:EntryPoint initialised with pybamm_models""" | ||
| model_sets = EntryPoint(group="pybamm_models") |
There was a problem hiding this comment.
this should just be called models, a parameter_set is a set of parameters (and a model is a set of equations), there is no set of models
There was a problem hiding this comment.
Yes that was intentional, models wasn't working because the pybamm.models namespace is already taken by the collection of battery models which gets overridden first. We could either change the variable name which was my temporary fix (model_sets) to something more sensible or we could change the namespace for the entry point models to something like pybamm.entrypoint.models. I'd prefer the first option though if you could come up with a sensible variable name for the models instance because I have a limited knowledge with the battery library.
There was a problem hiding this comment.
Understood! In order to keep it consistent with parameter sets let's keep it as is then
There was a problem hiding this comment.
How about moving both under pybamm.dispatch (internally, not exposed to users)? It's becoming fairly common with tools like networkx and scikit-image: https://scientific-python.org/specs/spec-0002/#projects-developing-a-python-entry_points-based-backend-dispatching
There was a problem hiding this comment.
Sorry for the late response. I have abstracted the models instance inside the dispatch namespace as @agriyakhetarpal mentioned. I did not move the parameter_sets instance inside the dispatch namespace, as it would be a breaking change and also fail a lot of tests because pybamm.parameter_sets wouldn't work instead, people would have to use pybamm.dispatch.parameter_sets, though both of these work as of now. The models instance (entry point) can now be accessed through pybamm.dispatch.models and list(pybamm.dispatch.models) would list all the battery models available through the entry point. To load the model, pybamm.Model("SPM") would return the model object.
santacodes
force-pushed
the
modelentrypoints
branch
from
faa78bc
to
164f71e
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
santacodes
changed the title
santacodes
force-pushed
the
modelentrypoints
branch
from
8a90a9b
to
8da0b01
Compare
Description
Modified the existing
parameter_setsAPI and unified entry points to accommodatemodel entry points. This is an extension of the existing model entry points in thepybamm-cookieproject. Relevant discussion.Fixes # (issue)
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: